🦋 Changeset detected

Latest commit: 914609b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
agents-eval-api	Ready	Preview, Comment	Jan 16, 2026 6:35pm
agents-manage-api	Ready	Preview, Comment	Jan 16, 2026 6:35pm
agents-manage-ui	Ready	Preview, Comment	Jan 16, 2026 6:35pm
agents-run-api	Ready	Preview, Comment	Jan 16, 2026 6:35pm

1 Skipped Deployment

Project	Deployment	Review	Updated (UTC)
agents-docs	Skipped		Jan 16, 2026 6:35pm

Claude finished @shagun-singh-inkeep's task —— View job

Code Review: Error Handling for Speakeasy MCPs

Todo List

• Read and understand the changed files
• Analyze the error handling implementation
• Review existing error handling patterns in the codebase
• Provide comprehensive architectural review feedback
• Check for security, performance, and best practices

Review Findings

High Priority Issues

1. Unsafe JSON Parsing (Security & Stability Risk)

// Line 846: Silent failure could mask errors
try { contentText = JSON.parse(rawResult?.content?.[0]?.text); } catch {}

Issues:

• Silent exception swallowing without logging hides potential parsing issues
• No validation of the parsed JSON structure
• Could lead to unexpected behavior when contentText remains undefined

Recommendation:

let contentText: any;
try {
  const rawText = rawResult?.content?.[0]?.text;
  if (typeof rawText === 'string') {
    contentText = JSON.parse(rawText);
  }
} catch (parseError) {
  logger.warn({ 
    toolName, 
    toolCallId, 
    parseError: parseError.message,
    rawText: rawResult?.content?.[0]?.text 
  }, 'Failed to parse MCP tool response as JSON');
}

2. Hardcoded HTTP Status Codes (Maintainability)

[400, 401, 403, 404, 422, 500].includes(contentText.StatusCode)

Issues:

• Magic numbers reduce readability and maintainability
• No clear rationale for why these specific codes were chosen
• Missing common error codes like 429 (rate limiting), 503 (service unavailable)

Recommendation:

const ERROR_STATUS_CODES = [
  400, // Bad Request
  401, // Unauthorized  
  403, // Forbidden
  404, // Not Found
  422, // Unprocessable Entity
  429, // Rate Limited
  500, // Internal Server Error
  503  // Service Unavailable
] as const;

const hasErrorInContent = 
  contentText?.StatusCode && 
  ERROR_STATUS_CODES.includes(contentText.StatusCode);

Medium Priority Issues

3. Type Safety & Validation

• No type definitions for the expected MCP response structure
• contentText is typed as any, reducing type safety
• No validation that StatusCode is actually a number

Recommendation: Define proper interfaces:

interface MCPToolResponse {
  StatusCode?: number;
  // other expected fields
}

interface MCPRawResult {
  isError?: boolean;
  content?: Array<{ text?: string }>;
}

4. Inconsistent Error Message Handling
The fallback error message construction could be simplified:

const errorMessage = typeof rawText === 'string' ? rawText : JSON.stringify(rawText) || 'MCP tool returned an error';

Recommendation:

const errorMessage = rawText || 'MCP tool returned an error';

Architecture & Design Considerations

5. Single Responsibility Principle
This error detection logic is becoming complex and could benefit from extraction:

private isErrorResult(rawResult: unknown): { isError: boolean; errorMessage: string } {
  // Extract all error detection logic here
}

6. Logging Strategy
Consider adding structured logging for different error scenarios:

if (hasExplicitError) {
  logger.error({ toolName, toolCallId, errorType: 'explicit' }, 'MCP tool explicit error');
} else if (hasErrorInContent) {
  logger.error({ toolName, toolCallId, errorType: 'status_code', statusCode: contentText.StatusCode }, 'MCP tool status code error');
}

Questions for Clarification

1. Speakeasy-specific Logic: Is the StatusCode checking specifically for Speakeasy MCPs, or should this be generalized for all MCPs?

2. Error Code Coverage: Are there other error status codes that should be included? Consider 429 (rate limiting) and 503 (service unavailable).

3. Backward Compatibility: Does this change affect existing MCPs that might have different response formats?

Positive Aspects

• Good separation of explicit vs implicit error detection
• Proper logging with structured context
• Maintains existing error handling flow

Testing Recommendations

Consider adding tests for:

• Invalid JSON in MCP responses
• Various HTTP status codes
• Edge cases where content array is empty or malformed